Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new config hook #1577

Merged
merged 12 commits into from
Jan 17, 2025
Merged

Add new config hook #1577

merged 12 commits into from
Jan 17, 2025

Conversation

bensmrs
Copy link
Contributor

@bensmrs bensmrs commented Jan 16, 2025

Closes #1400

@bensmrs bensmrs requested a review from stgraber as a code owner January 16, 2025 18:19
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Jan 16, 2025
@bensmrs
Copy link
Contributor Author

bensmrs commented Jan 16, 2025

And obviously I forgot to fix the tests :)

@bensmrs bensmrs force-pushed the qemu-scriptlet-config branch from 1159c52 to 1a431a4 Compare January 16, 2025 20:20
@bensmrs
Copy link
Contributor Author

bensmrs commented Jan 16, 2025

Phew that linter is very conservative!

@bensmrs bensmrs force-pushed the qemu-scriptlet-config branch from 1a431a4 to a85a512 Compare January 16, 2025 21:04
@stgraber
Copy link
Member

Looks good and was pretty easy to review thanks to how the commits were split.

Only nit is around those marshal/unmarshal and temporary config type in the scriptlet package.
I think it'd be better to them be part of the cfg package, but if that's impractical, then they should at least mention qemu in their name so someone looking at what's in the scriptlet package doesn't get confused.

@bensmrs
Copy link
Contributor Author

bensmrs commented Jan 17, 2025

but if that's impractical

It’s not, but the values marshalConf give and unmarshalConf take are a bit… wonky.
I’m not a huge fan of map[string]any values wandering through code for anything other than serialization. Here, any means either string or []string, which are very different to type-assert and deal with.
Also I don’t see why anyone would use these functions anywhere else when we have nicely typed structs.

they should at least mention qemu in their name

qemuMarshalConf, marshalQemuConf, or marshalQEMUConf? I prefer the 3rd.

@stgraber
Copy link
Member

qemuMarshalConf, marshalQemuConf, or marshalQEMUConf? I prefer the 3rd.

Yeah, 3rd is fine with me.

@bensmrs bensmrs force-pushed the qemu-scriptlet-config branch from a85a512 to be7d5f3 Compare January 17, 2025 21:19
@stgraber stgraber enabled auto-merge January 17, 2025 21:58
@stgraber stgraber merged commit 5220686 into lxc:main Jan 17, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

Allow QEMU scriptlets to read VM structs and edit QEMU config
2 participants